Nit: I think the better way to go is just to use the Python built-in dict, much as you already do for list and tuple. That means you don't need to import typing.Dict here. (When we don't have a big feature on the boil for HOSS, I'm going to go through and clean up the other modules that currently use typing.Dict, typing.List, etc, for consistency)

updated. - 1c29221

Do you really need this comment? It seems a bit superfluous.

updated - 1c29221

I know this is a comment on the previously written documentation string, but this description strongly implies the return of this function is a list containing a single latitude and longitude variable. Instead it returns a list with two sub-lists (one each of latitude and longitude variable names).

Also,

This function returns latitude and longitude variables...

probably would be clearer as:

This function returns latitude and longitude variable names...

(E.g., it specifically returns the names of the variables, not the variables themselves)

done - 1c29221

(Same comment for the similar change in the list comprehension for longitude_coordinate_variables)

Where do you catch any issues if there is a reference to a variable in a coordinates metadata attribute that isn't present in the file? If it's pointing to a latitude or longitude variable that isn't present in the file, at some point that's going to cause a failure when you try to access that variable. Is that gracefully handled somewhere?

(It's likely this is already nicely supported, but it's been a while since the previous PR and I just can't remember)

Separate comment - you have not added unit tests to prove this case is being handled as expected. (Needed both for latitude or longitude)

• this method is first called before prefetch. it should not fail if the variables don't exist.
• if it is present in the variable and not in the file - it will fail during prefetch. That is when it is first accessed from the file. - will check if I have a test on that
• There is a unit test for the function - will add subtests if the variables don't exist

There are unit tests to check for the above cases in line 205 of test_coordinate_utilities.py
https://github.com/nasa/harmony-opendap-subsetter/blob/57744a6298adf081d0bf06c9a6acd0b98cccf99c/tests/unit/test_coordinate_utilities.py#L205C1-L219C1

You can avoid declaring a variable for a single immediate use and just do:

return list(zip(points_x, points))

done. - 1c29221

This is another place where the in-line comments should go in the documentation string.

done - 2f07d1a

Nice tests. I think there are a couple of additional missing test-cases, though:

• There is only a single valid pixel.
• There are no valid pixels.

Possibly also what happens when there are only valid points in a single row or single column (for geographic things).

updated unit tests - a3680c5

Similar comments to a couple of other places. This unit test is only really testing the happy-path. You aren't really throwing any potential edge-cases at the function. Other test cases are:

• Only a single valid pixel.
• Only valid pixels in a single row.
• Only valid pixels in a single column.
• No valid pixels.

These seem like overkill, but it's really important to know how this code behaves in realistic edge-cases, so we know failure is graceful.

updated unit tests - 57744a6

Given that you've added conditional logic to the get_prefetch_variables function, you need to add more test-cases for it. The get_prefetch_variables function will need a subtest (or even separate tests if you prefer) that runs through each branch of code.

Right now, I think this test just ensures variables with dimensions, but not bounds, behave nicely. There's also test_prefetch_dimensions_with_bounds lower down in the file that ensures variables with dimensions and bounds work well. But there's nothing invoking prefetch_variables for requested variables with anonymous dimensions.

updated - 3b0a149